Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for vector in DataValueExt::int() #6844

Merged
merged 4 commits into from
Aug 14, 2023
Merged

Add support for vector in DataValueExt::int() #6844

merged 4 commits into from
Aug 14, 2023

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Aug 14, 2023 •
edited
Loading

Fixes #6827

@gurry gurry requested a review from a team as a code owner August 14, 2023 06:52
@gurry gurry requested review from cfallin and removed request for a team August 14, 2023 06:52
@gurry
Copy link
Contributor Author

gurry commented Aug 14, 2023 •
edited
Loading

DataValueExt::int() doesn't currently seem to have any tests. Please let me know if I should add any tests for this PR or for DataValueExt::int() in general.

Wonder if it would be a good idea to create a test from the .clif in #6827

@gurry
Copy link
Contributor Author

gurry commented Aug 14, 2023 •
edited
Loading

Just saw that the test already exists:

wasmtime/cranelift/filetests/filetests/runtests/simd-ineg.clif

Lines 30 to 35 in 30d086f

function %ineg_i64x2(i64x2) -> i64x2 {
block0(v0: i64x2):
v1 = ineg v0
return v1
}
; run: %ineg_i64x2([99 -10]) == [-99 10]

So I've enabled it for the interpreter.

@gurry
Format code with cargo fmt
5fadae7 
and also improve comment
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Aug 14, 2023
afonso360
afonso360 approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@afonso360 afonso360 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hey,

This looks great! Thanks for fixing this!

DataValueExt::int() doesn't currently seem to have any tests. Please let me know if I should add any tests for this PR or for DataValueExt::int() in general.

Yeah, we usually test the opcodes via clif tests instead of the internals, so enabling the ineg tests is great!

@afonso360 afonso360 added this pull request to the merge queue Aug 14, 2023
@gurry
Copy link
Contributor Author

gurry commented Aug 14, 2023

Thanks @afonso360

In order to test with an 8 byte vector input to DataValueExt::int() I tried adding the following test to simd-ineg.clif but it crashed with the below trace:

RUST_BACKTRACE=1 cargo test --test filetests
    Finished test [unoptimized + debuginfo] target(s) in 0.47s
     Running tests/filetests.rs ([...]\wasmtime\target\debug\deps\filetests-d08e13383c5d8a1e.exe)
slow: 0.026 filetests\filetests\wasm\aarch64-relaxed-simd.wat
thread 'worker #2' panicked at 'called `Option::unwrap()` on a `None` value', cranelift\codegen\src\isa\x64\lower\isle.rs:550:21
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library\std\src\panicking.rs:593
   1: core::panicking::panic_fmt
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library\core\src\panicking.rs:67
   2: core::panicking::panic
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library\core\src\panicking.rs:117
   3: enum2$<core::option::Option<cranelift_codegen::isa::x64::inst::args::Gpr> >::unwrap<cranelift_codegen::isa::x64::inst::args::Gpr>
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26\library\core\src\option.rs:935
   4: cranelift_codegen::isa::x64::lower::isle::impl$0::gpr_new
             at .\codegen\src\isa\x64\lower\isle.rs:550
   5: cranelift_codegen::isa::x64::lower::isle::generated_code::constructor_put_in_gpr<cranelift_codegen::machinst::isle::IsleContext<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst>,cranelift_codegen::isa::x64::X64Backend> >    
             at C:\Users\gurinder.singh\Projects\Personal\wasmtime_dev\wasmtime\target\debug\build\cranelift-codegen-3e0280ce8237aaeb\out\isle_x64.rs:2610
   6: cranelift_codegen::isa::x64::lower::isle::generated_code::constructor_lower<cranelift_codegen::machinst::isle::IsleContext<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst>,cranelift_codegen::isa::x64::X64Backend> >
             at C:\Users\gurinder.singh\Projects\Personal\wasmtime_dev\wasmtime\target\debug\build\cranelift-codegen-3e0280ce8237aaeb\out\isle_x64.rs:16695
   7: cranelift_codegen::isa::x64::lower::isle::lower
             at .\codegen\src\isa\x64\lower\isle.rs:64
   8: cranelift_codegen::isa::x64::lower::impl$1::lower
             at .\codegen\src\isa\x64\lower.rs:325
   9: cranelift_codegen::machinst::lower::Lower<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst> >::lower_clif_block<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst>,cranelift_codegen::isa::x64::X64Backend>             at .\codegen\src\machinst\lower.rs:743
  10: cranelift_codegen::machinst::lower::Lower<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst> >::lower<enum2$<cranelift_codegen::isa::x64::lower::isle::generated_code::MInst>,cranelift_codegen::isa::x64::X64Backend>
             at .\codegen\src\machinst\lower.rs:1067
  11: cranelift_codegen::machinst::compile::compile<cranelift_codegen::isa::x64::X64Backend>
             at .\codegen\src\machinst\compile.rs:40
  12: cranelift_codegen::isa::x64::X64Backend::compile_vcode
             at .\codegen\src\isa\x64\mod.rs:61
  13: cranelift_codegen::isa::x64::impl$1::compile_function
             at .\codegen\src\isa\x64\mod.rs:73
  14: cranelift_codegen::context::Context::compile_stencil
             at .\codegen\src\context.rs:146
  15: cranelift_codegen::context::Context::compile
             at .\codegen\src\context.rs:206
  16: cranelift_jit::backend::impl$2::define_function_with_control_plane
             at .\jit\src\backend.rs:689
  17: cranelift_filetests::function_runner::TestFileCompiler::define_function
             at .\filetests\src\function_runner.rs:248
  18: cranelift_filetests::function_runner::TestFileCompiler::add_functions
             at .\filetests\src\function_runner.rs:148
  19: cranelift_filetests::function_runner::TestFileCompiler::add_testfile
             at .\filetests\src\function_runner.rs:165
  20: cranelift_filetests::test_run::compile_testfile
             at .\filetests\src\test_run.rs:127
  21: cranelift_filetests::test_run::impl$0::run_target
             at .\filetests\src\test_run.rs:197
  22: cranelift_filetests::runone::run
             at .\filetests\src\runone.rs:102
  23: cranelift_filetests::concurrent::worker_thread::closure$0::closure$0
             at .\filetests\src\concurrent.rs:152
  24: std::panicking::try::do_call<cranelift_filetests::concurrent::worker_thread::closure$0::closure_env$0,enum2$<core::result::Result<core::time::Duration,anyhow::Error> > >
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26\library\std\src\panicking.rs:500
  25: std::panicking::try::do_catch<cranelift_filetests::concurrent::worker_thread::closure$0::closure_env$0,enum2$<core::result::Result<core::time::Duration,anyhow::Error> > >
  26: std::panicking::try<enum2$<core::result::Result<core::time::Duration,anyhow::Error> >,cranelift_filetests::concurrent::worker_thread::closure$0::closure_env$0>
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26\library\std\src\panicking.rs:464
  27: std::panic::catch_unwind<cranelift_filetests::concurrent::worker_thread::closure$0::closure_env$0,enum2$<core::result::Result<core::time::Duration,anyhow::Error> > >
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26\library\std\src\panic.rs:142
  28: cranelift_filetests::concurrent::worker_thread::closure$0
             at .\filetests\src\concurrent.rs:152
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
FAIL filetests\filetests\runtests\simd-ineg.clif: panicked in worker #2: called `Option::unwrap()` on a `None` value
1355 tests
Error: 1 failure
error: test failed, to rerun pass `--test filetests`

While the interpreter worked fine, it was the compiler that crashed. Is this expected?

Merged via the queue into bytecodealliance:main with commit 76f1cb2 Aug 14, 2023
@afonso360
Copy link
Contributor

afonso360 commented Aug 14, 2023 •
edited
Loading

While the interpreter worked fine, it was the compiler that crashed. Is this expected?

Well, sort of. Ideally we would be able to compile all clif code, but 64 bit SIMD support is very incomplete both in the backends and in the interpreter itself. The only architectures that support it are AArch64 and RISC-V (And neither of those fully support it!).

I think X86_64 has never supported it, so yeah I would expect it to crash there.

@gurry
Copy link
Contributor Author

gurry commented Aug 14, 2023

Okay, makes sense. Thanks 👍

@gurry gurry deleted the issue-6827 branch August 16, 2023 02:27
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Aug 18, 2023
@gurry @eduardomourar
Add support for vector in DataValueExt::int() (bytecodealliance#6844)
e3af3d8 
* Add support for vector in DataValueExt::int()

Fixes bytecodealliance#6827

* Replace `if` with `match`

* Enable interpreter test for simd ineg

Issue bytecodealliance#6827

* Format code with `cargo fmt`

and also improve comment
geekbeast pushed a commit to geekbeast/wasmtime that referenced this pull request Aug 18, 2023
Merge branch 'feature/wasi-nn-preview-2' of github.com:geekbeast/wasm…
df3cef9 
…time into feature/wasi-nn-preview-2

* 'feature/wasi-nn-preview-2' of github.com:geekbeast/wasmtime:
  Memcheck for Wasm guests in Wasmtime (bytecodealliance#6820)
  CI: upgrade to qemu 8.0.4. (bytecodealliance#6849)
  Sync wasi-cli with wit definitions in standards repo  (bytecodealliance#6806)
  Rename `preview2::preview2` to `preview2::host` (bytecodealliance#6847)
  winch: Simplify the MacroAssembler and Assembler interfaces (bytecodealliance#6841)
  There are no files in `preview1` other than `mod.rs` (bytecodealliance#6845)
  Update stdio on Unix to fall back to worker threads (bytecodealliance#6833)
  Update RELEASES.md (bytecodealliance#6838)
  Minor documentation updates to docs/WASI-tutorial.md (bytecodealliance#6839)
  Add support for vector in DataValueExt::int() (bytecodealliance#6844)
@afonso360 afonso360 mentioned this pull request Aug 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afonso360 afonso360 afonso360 approved these changes

@cfallin cfallin Awaiting requested review from cfallin cfallin is a code owner automatically assigned from bytecodealliance/wasmtime-compiler-reviewers

Assignees
No one assigned
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cranelift: SIMD ineg crashes the interpreter
2 participants
@gurry @afonso360